-
-
Notifications
You must be signed in to change notification settings - Fork 706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed TextDisplay's offsets. #4676
Conversation
core/src/main/java/org/geysermc/geyser/entity/type/DisplayBaseEntity.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/geysermc/geyser/entity/EntityDefinitions.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/geysermc/geyser/entity/type/DisplayBaseEntity.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/geysermc/geyser/entity/type/DisplayBaseEntity.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/geysermc/geyser/entity/type/DisplayBaseEntity.java
Outdated
Show resolved
Hide resolved
Why is this still not approved? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you go - after these, this should be good to merge. If you have questions about how to implement specifics, feel free to reach out via discord.
core/src/main/java/org/geysermc/geyser/entity/EntityDefinitions.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/geysermc/geyser/entity/type/DisplayBaseEntity.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/geysermc/geyser/entity/type/DisplayBaseEntity.java
Outdated
Show resolved
Hide resolved
Looks better now, but if you're alright with that, I'd push a few minor changes to this PR myself - I'm pretty sure that we could cancel the entity mount offset from getting applied in a way that doesn't involve adding an extra check to that static method. |
It was messing with the offsets until I added that check, it seems like the function is triggering before If you can't fix it quickly it's best to leave the safety check, this check is not being called that often to mess with performance even at 10k TextDisplay passengers (tested). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed some smaller cleanup changes (codestyle fixes, removed the hasTranslation variable) - just one final thing, and then this should be good to be merged. Sorry for the wait!
core/src/main/java/org/geysermc/geyser/entity/type/DisplayBaseEntity.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay now. Thanks!
Fixed TextDisplay's offsets by adding the Translation (https://wiki.vg/Entity_metadata#Entity_Metadata_Format), mainly for passenger entities.